Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(Makefile): add fetch tags action #2167

Closed

Conversation

vgonkivs
Copy link
Member

@vgonkivs vgonkivs commented May 5, 2023

Overview

Added fetching tags action in order to get the correct semantic version.

Checklist

  • New and updated code has appropriate documentation
  • New and updated code has new and/or updated testing
  • Required CI checks are passing
  • Visual proof for any user facing features like CLI or documentation updates
  • Linked issues closed with keywords

@vgonkivs vgonkivs self-assigned this May 5, 2023
@vgonkivs vgonkivs added the kind:feat Attached to feature PRs label May 5, 2023
Copy link
Member

@Wondertan Wondertan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fetch-tags is effectively an alias for git fetch --tags. Is it worth adding a make command for it? We basically write fetch-tags instead of git fetch --all and this feels like an overkill and Makefile bloat

Makefile Outdated Show resolved Hide resolved
Copy link
Member

@renaynay renaynay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wouldn't add fetch-tags as part of the other make targets - lts just keep it separate

Also +1 to hlib comment re all remotes

derrandz
derrandz previously approved these changes May 5, 2023
Copy link
Contributor

@derrandz derrandz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Although it might seem as Makefile bloat, having this as a target allows referencing it in other targets easily.

Makefile Outdated
@@ -17,7 +17,7 @@ install-hooks:
.PHONY: init-hooks

## build: Build celestia-node binary.
build:
build: fetch-tags
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you remove fetch-tags from every other target?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should the build only contain it?

Then it is no sense to have target

@Wondertan
Copy link
Member

Wondertan commented May 5, 2023

allows referencing it in other targets easily.

Other targets can call git directly if they need to.

@vgonkivs vgonkivs force-pushed the add_fetch_tags_to_makefile branch 3 times, most recently from 73f3c2b to 9a0d544 Compare May 5, 2023 17:09
@vgonkivs
Copy link
Member Author

vgonkivs commented May 5, 2023

blocked by celestiaorg/.github#58

@vgonkivs vgonkivs force-pushed the add_fetch_tags_to_makefile branch 8 times, most recently from 9fab3dc to 5d6caca Compare May 6, 2023 09:58
@vgonkivs vgonkivs force-pushed the add_fetch_tags_to_makefile branch 4 times, most recently from 1a2e501 to 441da7e Compare May 6, 2023 10:31
@codecov-commenter
Copy link

codecov-commenter commented May 6, 2023

Codecov Report

Merging #2167 (45a0942) into main (5c0b128) will increase coverage by 0.52%.
The diff coverage is 73.19%.

@@            Coverage Diff             @@
##             main    #2167      +/-   ##
==========================================
+ Coverage   55.44%   55.96%   +0.52%     
==========================================
  Files         212      213       +1     
  Lines       13576    13865     +289     
==========================================
+ Hits         7527     7760     +233     
- Misses       5286     5332      +46     
- Partials      763      773      +10     
Impacted Files Coverage Δ
cmd/celestia/rpc.go 11.44% <0.00%> (-0.35%) ⬇️
nodebuilder/node.go 57.35% <0.00%> (ø)
nodebuilder/share/opts.go 62.50% <57.14%> (-37.50%) ⬇️
share/p2p/shrexeds/client.go 64.00% <57.14%> (-0.52%) ⬇️
share/p2p/shrexeds/params.go 62.50% <57.14%> (-7.50%) ⬇️
share/p2p/shrexnd/params.go 62.50% <57.14%> (-37.50%) ⬇️
share/p2p/shrexnd/server.go 66.89% <68.75%> (-0.03%) ⬇️
share/getters/shrex.go 81.25% <70.00%> (-4.33%) ⬇️
share/p2p/metrics.go 70.96% <70.96%> (ø)
share/p2p/shrexnd/client.go 68.00% <71.42%> (-0.34%) ⬇️
... and 10 more

... and 7 files with indirect coverage changes

@vgonkivs vgonkivs force-pushed the add_fetch_tags_to_makefile branch 4 times, most recently from 1635b52 to a030356 Compare May 6, 2023 11:09
@vgonkivs vgonkivs force-pushed the add_fetch_tags_to_makefile branch from a030356 to e9d8685 Compare May 6, 2023 11:13
@@ -17,6 +17,6 @@ jobs:
permissions:
contents: write
packages: write
uses: celestiaorg/.github/.github/workflows/reusable_dockerfile_pipeline.yml@v0.1.1 # yamllint disable-line rule:line-length
uses: celestiaorg/.github/.github/workflows/reusable_dockerfile_pipeline.yml@test_context # yamllint disable-line rule:line-length
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

test branch w modified reusable_dockerfile_pipeline

@vgonkivs
Copy link
Member Author

vgonkivs commented May 8, 2023

N/A

@vgonkivs vgonkivs closed this May 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind:feat Attached to feature PRs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants